Overload: Limit the number of streams reset per invocation of WatermarkBufferFactory::resetAccountsGivenPressure.#17949
Merged
alyssawilk merged 2 commits intoenvoyproxy:mainfrom Sep 7, 2021
Conversation
WatermarkBufferFactory::resetAccountsGivenPressure. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Contributor
Author
|
/assign @alyssawilk |
alyssawilk
reviewed
Sep 2, 2021
Contributor
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Nice! Looks good modulo 2 tiny nits.
/wait
| // Effectively disables tracking as this should zero out all reasonable account | ||
| // balances when shifted by this amount. | ||
| constexpr uint32_t kEffectivelyDisableTrackingBitshift = 63; | ||
| constexpr uint32_t kMaxNumberOfStreamsToResetPerInvocation = 50; |
Contributor
There was a problem hiding this comment.
I'd comment here as well that 50 is arbitrary, to limit work done (as you doc up above)
| no streams will be reset. When heap usage is at or above 85%, we start to | ||
| reset buckets according to the strategy described below. When the heap | ||
| usage is at 95% all streams using >= 1MiB memory are eligible for reset. | ||
| This Overload action will reset up to 50 streams (this is a hardcoded limit) |
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
KBaichoo
commented
Sep 2, 2021
Contributor
Author
KBaichoo
left a comment
KBaichoo
left a comment
There was a problem hiding this comment.
Thanks for the review @alyssawilk
| // Effectively disables tracking as this should zero out all reasonable account | ||
| // balances when shifted by this amount. | ||
| constexpr uint32_t kEffectivelyDisableTrackingBitshift = 63; | ||
| constexpr uint32_t kMaxNumberOfStreamsToResetPerInvocation = 50; |
| no streams will be reset. When heap usage is at or above 85%, we start to | ||
| reset buckets according to the strategy described below. When the heap | ||
| usage is at 95% all streams using >= 1MiB memory are eligible for reset. | ||
| This Overload action will reset up to 50 streams (this is a hardcoded limit) |
alyssawilk
approved these changes
Sep 7, 2021
tyxia
pushed a commit
to tyxia/envoy
that referenced
this pull request
Sep 21, 2021
…rkBufferFactory::resetAccountsGivenPressure. (envoyproxy#17949) Commit Message: Limit the number of streams reset per invocation of WatermarkBufferFactory::resetAccountsGivenPressure. Additional Description: Since we limit number of streams reset, we should start resetting the largest eligible streams first. Risk Level: low Testing: unit test Docs Changes: Included Release Notes: TBD Platform Specific Features: NA Related Issue envoyproxy#15791 Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Kevin Baichoo kbaichoo@google.com
Commit Message: Limit the number of streams reset per invocation of WatermarkBufferFactory::resetAccountsGivenPressure.
Additional Description: Since we limit number of streams reset, we should start resetting the largest eligible streams first.
Risk Level: low
Testing: unit test
Docs Changes: Included
Release Notes: TBD
Platform Specific Features: NA
Related Issue #15791